-
-
Notifications
You must be signed in to change notification settings - Fork 929
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor atrule statement in unit-no-unknown #5515
Refactor atrule statement in unit-no-unknown #5515
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sylwia-lask Thanks for creating the PR! I've commented on a refactoring suggestion, so can you check it out?
lib/rules/unit-no-unknown/index.js
Outdated
@@ -129,7 +130,7 @@ function rule(actual, options) { | |||
} | |||
|
|||
root.walkAtRules((atRule) => { | |||
if (!/^media$/i.test(atRule.name) && !atRule.variable) { | |||
if (!/^media$/i.test(atRule.name) && !isStandardSyntaxAtRule(atRule)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[suggestion] Can we move the position of /^media$/i
as below? I think it's more readable. 😃
root.walkAtRules(/^media$/i, (atRule) => {
if (!isStandardSyntaxAtRule(atRule)) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, we should be using the walk*
filter when available. I think most rules do, but it looks like this one must have slipped through at some point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ybiquitous @jeddy3 thanks a lot guys for your review :)
5d49cdf
to
d93de00
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sylwia-lask Thanks. LGTM! 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sylwia-lask Looks great, thank you!
PR is related to discussion in #5500 - introduce isStandardAtRule.
No, it's self-explanatory.